Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New module plan_stash #113

Merged
merged 8 commits into from
Feb 23, 2024
Merged

Conversation

abikouo
Copy link
Contributor

@abikouo abikouo commented Feb 2, 2024

SUMMARY

A module to perform base64-encoding and set_stats for a terraform plan file. This will be useful to pass artifacts between workflows in AAP

ISSUE TYPE
  • New Module Pull Request

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (stable-plan-stash@1790eb3). Click here to learn what that means.

Additional details and impacted files
@@                 Coverage Diff                  @@
##             stable-plan-stash     #113   +/-   ##
====================================================
  Coverage                     ?   74.10%           
====================================================
  Files                        ?       17           
  Lines                        ?     1058           
  Branches                     ?      186           
====================================================
  Hits                         ?      784           
  Misses                       ?      243           
  Partials                     ?       31           
Flag Coverage Δ
units 74.10% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Copy link

Copy link
Contributor

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See one comment about the action plugin, but generally this looks good and works as expected.

plugins/action/plan_stash.py Outdated Show resolved Hide resolved
Copy link

Copy link
Contributor

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

plugins/modules/plan_stash.py Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this, I'm realizing we may want to make this module work both for stashing and for loading the plan file. The only way to safely write the b64 encoded data to a zip file would be for the user to have a shell task that calls out to base64, which is kind of ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gravesm it makes sense to support the load of the plan file, but currently, a shell task is not the only way to do that, we can do it using ansible.builtin.copy module

- copy:
    dest: /path/to/plan/file
    content: "{{ terraform_plan | b64decode }}"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear to me that this is always going to work, though. The documentation for the b64decode filter pretty clearly says this is likely to corrupt the binary data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I am going to update the module to work like this

Stash the terraform plan file

- cloud.terraform.plan_stash:
     mode: stash
     path: terraform.tfplan
     var_name: terraform_plan

Load the terraform plan file

- cloud.terraform.plan_stash
    mode: load
    path: terraform.tfplan
    var_name: terraform_plan

WDYT ? @gravesm @hakbailey

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is along the lines of what I'm thinking. My only suggestion would be to use state instead of mode.

Copy link

Copy link

Copy link
Contributor

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the addition of the load option! See some minor comments in the documentation block. I also wonder if we want to note something about how set_stats does not set variables in the way set_facts does during local ansible-playbook runs...basically explaining that this module is intended for use within an AAP workflow.

plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
Copy link

@@ -29,7 +29,7 @@ commands =

[testenv:black]
deps =
black
black==23.12.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should it be tied to this specific version of black?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because a new release of black may generate formatting errors.

Copy link

plugins/modules/plan_stash.py Show resolved Hide resolved
plugins/modules/plan_stash.py Outdated Show resolved Hide resolved
@abikouo abikouo requested a review from gravesm February 21, 2024 16:52
Copy link

@abikouo abikouo changed the base branch from stable-plan-stash to main February 23, 2024 06:05
@abikouo abikouo merged commit c70ff08 into ansible-collections:main Feb 23, 2024
38 checks passed
Copy link

patchback bot commented May 14, 2024

Backport to stable-2: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-2/c70ff08503bb636f46edd61714dd545dcaa8180b/pr-113

Backported as #127

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request May 14, 2024
* Using an action plugin

* add integration tests for the plan_stash module

* Update default plan stash variable to terraform_plan

* Update action module with ansible arg specs validation using validate_argument_spec() function

* add note to suggest use of no_log: true

* Add new feature allowing to load the terraform file

* Fix sanity tests

* minor refactoring

(cherry picked from commit c70ff08)
gravesm pushed a commit that referenced this pull request May 14, 2024
* Using an action plugin

* add integration tests for the plan_stash module

* Update default plan stash variable to terraform_plan

* Update action module with ansible arg specs validation using validate_argument_spec() function

* add note to suggest use of no_log: true

* Add new feature allowing to load the terraform file

* Fix sanity tests

* minor refactoring

(cherry picked from commit c70ff08)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request May 14, 2024
[PR #113/c70ff085 backport][stable-2] New module plan_stash

This is a backport of PR #113 as merged into main (c70ff08).
SUMMARY

A module to perform base64-encoding and set_stats for a terraform plan file. This will be useful to pass artifacts between workflows in AAP

ISSUE TYPE


New Module Pull Request

Reviewed-by: Alina Buzachis
Reviewed-by: Mike Graves <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants